Get telemetry probes in place that will let us monitor regressions such as bug 959130.

Once bug 936630 has landed, we will have the following timeline events: - sessionRestoreInit - sessionRestoreInitialized - sessionRestoreRestoring - sessionRestoreRestored. I believe that we need the following data: - how long it takes to read sessionstore.js (we already have it as FX_SESSION_RESTORE_READ_FILE_MS); - how long it takes to parse sessionstore.js; - how long it takes to process it; - how long it takes to perform all DOM operations that are required before the user can browse (sessionRestoreRestored - sessionRestoreInit).

Comment on attachment 8417306 [details] [diff] [review] More telemetry on session restore startup Review of attachment 8417306 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionStore.jsm @@ +759,5 @@ > // ignore windows opened while shutting down > if (this._loadState == STATE_QUITTING) > return; > > + try { OMG no, we can't do that. We should not wrap all that in try/catch and at the same time throw away all blame info. We have a real problem if we throw in onLoad() anyway. @@ +760,5 @@ > if (this._loadState == STATE_QUITTING) > return; > > + try { > + TelemetryStopwatch.start("FX_SESSION_RESTORE_STARTUP_PERFORM_DELAYED_RESTORATION_MS"); onLoad() is called for each window that we do or do not restore into. Maybe measure the .onLoad() call in onOpen() if |initialState| is not null? ::: toolkit/components/telemetry/Histograms.json @@ +3343,5 @@ > + "kind": "exponential", > + "high": "30000", > + "n_buckets": 20, > + "extended_statistics_ok": true, > + "description": "Session restore: Time it takes to finish restoration once we have first openeed a window (ms)" *opened

(In reply to Tim Taubert [:ttaubert] from comment #3) > Comment on attachment 8417306 [details] [diff] [review] > More telemetry on session restore startup > > Review of attachment 8417306 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/sessionstore/src/SessionStore.jsm > @@ +759,5 @@ > > // ignore windows opened while shutting down > > if (this._loadState == STATE_QUITTING) > > return; > > > > + try { > > OMG no, we can't do that. We should not wrap all that in try/catch and at > the same time throw away all blame info. We have a real problem if we throw > in onLoad() anyway. Hey, where do you see a `catch` in my code? :)

Same one, without bits of the next patch.

(and again)

Comment on attachment 8417399 [details] [diff] [review] More telemetry on session restore startup, v4 Review of attachment 8417399 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionStore.jsm @@ +815,5 @@ > // The next delayed save request should execute immediately. > SessionSaver.clearLastSaveTime(); > } > + } else if (!this._isWindowLoaded(aWindow)) { > + // this window was opened by _openWindowWithState I'd rather have us not do these tiny cleanups and keep the blame info. @@ +979,5 @@ > + > + this.onLoad(aWindow, initialState); > + } finally { > + TelemetryStopwatch.finish("FX_SESSION_RESTORE_STARTUP_PERFORM_FIRST_LOAD_MS"); > + } No need to try/finally this. ::: toolkit/components/telemetry/Histograms.json @@ +3333,5 @@ > + "FX_SESSION_RESTORE_STARTUP_INIT_RESTORATION_MS": { > + "expires_in_version": "never", > + "kind": "exponential", > + "high": "30000", > + "n_buckets": 120, Did you mean 120 buckets? 20 should be enough?

Comment on attachment 8417419 [details] [diff] [review] More telemetry on session restore startup, v5 Review of attachment 8417419 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionStore.jsm @@ +384,5 @@ > /** > * Initialize the session using the state provided by SessionStartup > */ > initSession: function () { > + TelemetryStopwatch.start("FX_SESSION_RESTORE_STARTUP_INIT_RESTORATION_MS"); Can you please call this ...INIT_SESSION_MS? @@ +979,5 @@ > } else { > let initialState = this.initSession(); > this._sessionInitialized = true; > + > + TelemetryStopwatch.start("FX_SESSION_RESTORE_STARTUP_PERFORM_FIRST_LOAD_MS"); This maybe... FX_SESSION_RESTORE_STARTUP_ONLOAD_INITIAL_WINDOW_MS? ::: toolkit/components/telemetry/Histograms.json @@ +3333,5 @@ > + "FX_SESSION_RESTORE_STARTUP_INIT_RESTORATION_MS": { > + "expires_in_version": "never", > + "kind": "exponential", > + "high": "30000", > + "n_buckets": 120, Is that 120 buckets intentional or not?

> ::: toolkit/components/telemetry/Histograms.json > @@ +3333,5 @@ > > + "FX_SESSION_RESTORE_STARTUP_INIT_RESTORATION_MS": { > > + "expires_in_version": "never", > > + "kind": "exponential", > > + "high": "30000", > > + "n_buckets": 120, > > Is that 120 buckets intentional or not? My bad, I thought I had removed that.

Comment on attachment 8417455 [details] [diff] [review] More telemetry on session restore startup, v6 Review of attachment 8417455 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionStore.jsm @@ +395,1 @@ > } The bad thing here is that even with a broken state (invalid session file that's not parseable) we can end up with ss.doRestore() returning true. We either need the try here or fix nsSessionStartup to check |this._initialState| and set sessionType to NO_SESSION. I'd very much prefer the latter. We should also have an xpcshell test similar to test_startup_invalid_session.js that sets browser.startup.page=3 and checks that we still get NO_SESSION with an unparseable file.

Is that the right bug to handle this issue? I simply removed a try/catch block that can't raise.

Comment on attachment 8417455 [details] [diff] [review] More telemetry on session restore startup, v6 Review of attachment 8417455 [details] [diff] [review]: ----------------------------------------------------------------- Ignore me, sorry.

According to http://raluca-elena.github.io/telemetry-dashboard/#filter=nightly%2F32%2FFX_SESSION_RESTORE_READ_FILE_MS!nightly%2F32%2FFX_SESSION_RESTORE_STARTUP_INIT_SESSION_MS!nightly%2F32%2FFX_SESSION_RESTORE_STARTUP_ONLOAD_INITIAL_WINDOW_MS&aggregates=95th%20percentile&evoOver=Builds&locked=false&sanitize=true, on Nightly 32, FX_SESSION_RESTORE_READ_FILE_MS is by far the dominant cost over FX_SESSION_RESTORE_READ_FILE_MS and FX_SESSION_RESTORE_STARTUP_INIT_SESSION_MS. This suggests that we should concentrate our efforts on reading faster, i.e.: - reading earlier; and/or - making the file smaller.